Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

buildkite-agent env has --from-env-file and --print flags #1803

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pda
Copy link
Member

@pda pda commented Oct 25, 2022

Adds two flags to the buildkite-agent env command introduced in #1781.

By default, buildkite-agent env loads os.Environ() and prints it as a JSON object; it's mainly intended for internal use to capture a snapshot of environment before a hook runs, and detect changes afterwards.

The new --from-env-file boolean flag sources the environment from the file named by $BUILDKITE_ENV_FILE instead of from os.Environ(), using bufio.Scanner and strconv.Unquote() to safely and correctly parse the quoting/escaping done by %q when the file was written:

if _, err := r.envFile.WriteString(fmt.Sprintf("%s=%q\n", key, value)); err != nil {

The new --print NAME flag causes a single environment value to be printed in its raw unescaped form followed by a newline, instead of printing all vars as a JSON object.

Combined, this solves the problem of safely parsing BUILDKITE_ENV_FILE (which cannot be safely evaluated in a shell if it may contain untrusted user input) to access a single variable, and allows for usage like this in a pre-command hook or similar:

if [[ $(buildkite-agent env --from-env-file --print BUILDKITE_BRANCH) != "main" ]]; then
  echo "This agent only builds branch 'main'"
  exit 42
fi

@pda pda requested a review from a team October 25, 2022 10:04
Copy link
Contributor

@triarius triarius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but someone will likely have merge conficts.

for _, e := range env {
k, v, _ := strings.Cut(e, "=")
envMap[k] = v
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect @moskyb's efforts to also use this command in windows will cause a merge conflict here.

This solves the problem of safely parsing BUILDKITE_ENV_FILE, and allows
for usage like this in a pre-command hook or similar:

    local branch
    branch="$(buildkite-agent env --from-env-file --print BUILDKITE_BRANCH)"
    if [[ branch != "main" ]]; then
        echo "This agent only builds branch 'main'"
        exit 42
    fi
@pda pda force-pushed the buildkite-env-can-load-env-file-and-print-single-value branch from 2aaefb6 to a5cc6a9 Compare October 26, 2022 06:33
@pda pda changed the title buildkite-agent env has --env-file (bool) and --print NAME flags buildkite-agent env has --from-env-file and --print flags Oct 26, 2022
Comment on lines +51 to +52
if c.Bool("from-env-file") {
envMap, err = loadEnvFile(os.Getenv("BUILDKITE_ENV_FILE"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels weird to me. I expect --from-env-file to take a value, but default to an environment variable, like our other options. But if we defaulted to the env file then it would break dumping of the current env as json.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. I like your idea of moving the current “dump entire process env as JSON” use-cases into a subcommand/flag(s), which then frees up the buildkite-agent env namespace to support some other environment-related use-cases.

@sj26
Copy link
Member

sj26 commented Oct 30, 2022

This feels like the same domain but a pretty orthogonal use case to the underlying command to dump the current env as json. I think it should be a separate command. Maybe something like:

buildkite-agent env get NAME

But it's unclear to me then which env it's getting. Build env? Job env? The current process? The env file, pointed at by an env (inception moment)? I would presume the current process env. Which feels odd, because I can $BLAH.

Playing with some usage ...

buildkite-agent env get NAME # current process env?
buildkite-agent env --env-file get NAME # from $BUILDKITE_ENV_FILE?
buildkite-agent env --env-file=... get NAME
buildkite-agent env --json-file=... get NAME

???

Maybe the recently introduced command should also be a subcommand?

buildkite-agent env dump --json

@pda
Copy link
Member Author

pda commented Nov 8, 2022

Maybe the recently introduced command should also be a subcommand?

buildkite-agent env dump --json

Yeah I like that. Since the recently-added buildkite-agent env is private/internal usage only (hopefully), we're free to evolve it to support other use-cases around it in a coherent way 👍🏼

@sj26
Copy link
Member

sj26 commented Nov 8, 2022

The nice thing about subcommands is if we peer forwards to being able to set env from things which are not bash hooks, that could use buildkite-agent env set NAME VALUE to talk to the parent process and inject env without sourcing and env dumping shenanigans at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants